Revert "cmd/openshift-install/create: Allow hung watch" #741
Conversation
This reverts commit 6dc1bf6, openshift#615. 109531f (cmd/openshift-install/create: Retry watch connections, 2018-11-02, openshift#606) made the watch re-connects reliable, so make watch timeouts fatal again. This avoids confusing users by showing "Install complete!" messages when they may actually have a hung bootstrap process.
openshift-ci-robot
added
the
size/XS
openshift-ci-robot
added
the
approved
| if err != nil { | ||
| logrus.Error(errors.Wrap(err, "waiting for bootstrap-complete")) | ||
| return nil | ||
| return errors.Wrap(err, "waiting for bootstrap-complete") |
There was a problem hiding this comment.
to compare it to how the error would have been structured without wrap
fmt.Errorf("failed waiting for bootstrap-complete: %v", err)
I would suggest we keep it errors.Wrap(err, "failed waiting for bootstrap-complete")
There was a problem hiding this comment.
$ git describe --dirty
v0.4.0-25-gb13b7fa-dirty
$ git diff
diff --git a/cmd/openshift-install/create.go b/cmd/openshift-install/create.go
index 5ba6517..8c95b8c 100644
--- a/cmd/openshift-install/create.go
+++ b/cmd/openshift-install/create.go
@@ -203,8 +203,8 @@ func destroyBootstrap(ctx context.Context, directory string) (err error) {
watcher, err := events.Watch(metav1.ListOptions{
ResourceVersion: sinceResourceVersion,
})
- if err == nil {
- return watcher, nil
+ if err == nil || true {
+ return watcher, err
}
select {
case <-eventContext.Done():
$ TAGS=libvirt_destroy hack/build.sh
$ openshift-install --dir=wking create cluster
INFO Fetching OS image...
INFO Using Terraform to create cluster...
INFO Waiting for bootstrap completion...
INFO API v1.11.0+d4cacc0 up
WARNING RetryWatcher - getting event failed! Re-creating the watcher. Last RV: 2214
FATAL Error executing openshift-install: waiting for bootstrap-complete: watch closed before UntilWithoutRetry timeout That line includes both "FATAL" and "Error" already. Personally I think that's enough without also including an additional "failed", but I can add it if you like.
There was a problem hiding this comment.
as error returns from this function first one is more clear when looking at the function return (not few levels up).
I've since cleaned up the leaked CI VPCs, and #740 is in flight to address the current leak. /retest |
I'll wait until CI settles down and kick this again tonight. |
|
don't care enough about #741 (comment) (just preferred the recommendation) /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 participants
This reverts commit 6dc1bf6, #615.
109531f (#606) made the watch re-connects reliable, so make watch timeouts fatal again. This avoids confusing users by showing "Install complete!" messages when they may actually have a hung bootstrap process.